Skip to content

Allow selectcols to have tuples as "indices" argument - #992

Merged
ablaom merged 5 commits into
devfrom
selectcols
Mar 24, 2025
Merged

ablaom merged 5 commits into
devfrom
selectcols

Conversation

@ablaom

@ablaom ablaom commented Jan 5, 2025

Copy link
Copy Markdown
Member

This PR closes #991. It also fixes an unrelated test that seems to have been broken by recent Julia update. Something about the behaviour of @doc, it seems.

@OkonSamuel Let me know if you had a better suggestion for this PR. Otherwise, please review.

@ablaom
ablaom requested a review from OkonSamuel January 5, 2025 23:58
@ablaom

ablaom commented Mar 24, 2025

Copy link
Copy Markdown
Member Author

@OkonSamuel Any chance you could take a look at this?

@ablaom

ablaom commented Mar 24, 2025

Copy link
Copy Markdown
Member Author

@MilesCranmer Would you be able to review this?

Comment thread test/interface/data_utils.jl Resolved
@MilesCranmer

Copy link
Copy Markdown
Contributor

Apart from that it seems good to me

Comment thread src/interface/data_utils.jl Resolved
@ablaom

ablaom commented Mar 24, 2025 •

Copy link
Copy Markdown
Member Author

Thanks @MilesCranmer for the lightning-speed review. Outside reviews are essential for maintaining quality of the software and I appreciate them very much.

I've adopted your good suggestions.

@ablaom
ablaom merged commit 0a82ccd into dev Mar 24, 2025
@ablaom
ablaom deleted the selectcols branch March 24, 2025 22:47
@devmotion

Copy link
Copy Markdown
Contributor

FYI this PR - and hence the 1.8.0 release - are broken: Vararg{<:Integer} must be replaced with Vararg{Integer}:

ERROR: LoadError: Wrapping `Vararg` directly in UnionAll is deprecated (wrap the tuple instead).
You may need to write `f(x::Vararg{T})` rather than `f(x::Vararg{<:T})` or `f(x::Vararg{T}) where T` instead of `f(x::Vararg{T} where T)`.
Stacktrace:
 [1] UnionAll(v::Any, t::Any)
   @ Core ./boot.jl:299
 [2] top-level scope
   @ ~/.julia/packages/MLJBase/5aKTB/src/interface/data_utils.jl:133
 [3] include(mod::Module, _path::String)
   @ Base ./Base.jl:557
 [4] include(x::String)
   @ MLJBase ~/.julia/packages/MLJBase/5aKTB/src/MLJBase.jl:1
 [5] top-level scope
   @ ~/.julia/packages/MLJBase/5aKTB/src/MLJBase.jl:149
 [6] include
   @ ./Base.jl:557 [inlined]
 [7] include_package_for_output(pkg::Base.PkgId, input::String, depot_path::Vector{String}, dl_load_path::Vector{String}, load_path::Vector{String}, concrete_deps::Vector{Pair{Base.PkgId, UInt128}}, source::String)
   @ Base ./loading.jl:2881
 [8] top-level scope
   @ stdin:6
in expression starting at /build/.julia/packages/MLJBase/5aKTB/src/interface/data_utils.jl:133
in expression starting at /build/.julia/packages/MLJBase/5aKTB/src/MLJBase.jl:1
in expression starting at stdin:6
ERROR: LoadError: Failed to precompile MLJBase [a7f614a8-145f-11e9-1d2a-a57a1082229d] to "/build/.julia/compiled/v1.11/MLJBase/jl_KDRdpO".
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

selectcols should handle tuple input

3 participants